fix(fetch): raise AbortSignal max event listeners #1910

KhafraDev · 2023-02-06T18:02:54Z

Fixes nodejs/node#46525
Refs #939 (comment)

codecov-commenter · 2023-02-06T18:08:04Z

Codecov Report

Base: 90.41% // Head: 90.42% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (7615308) compared to base (9dceb21).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1910   +/-   ##
=======================================
  Coverage   90.41%   90.42%           
=======================================
  Files          71       71           
  Lines        6096     6099    +3     
=======================================
+ Hits         5512     5515    +3     
  Misses        584      584

Impacted Files	Coverage Δ
lib/fetch/request.js	`87.98% <100.00%> (+0.15%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

mcollina

Lgtm

ricardocosta · 2023-03-08T10:21:53Z

@KhafraDev According to remix-run/react-router#10118, this PR introduces errors when using React Router because getEventListeners doesn't support signals of type AbortSignal , whilst signal can be of that type.

Is there any issue/plan to fix this?

KhafraDev · 2023-03-08T15:21:33Z

getEventListeners does work on a nodejs AbortSignal, since it extends EventTarget. If you have a minimal repro that would be helpful.

joshkel · 2023-03-10T21:27:45Z

@KhafraDev, thank you for looking into this. However, I'm still having problems; depending on how quickly my loop runs, Node.js might add >200 event listeners, so I still get these warnings spammed to the console. I tried raising the warning limit myself (setMaxListeners(Infinity, this.controller.signal)); however, this PR overrides any warning limit that was already set.

For a repro for remix-run/react-router#10118, see https://github.com/joshkel/vitest-undici-abort-bug. The problem arises because testing frameworks such as Vitest may cause jsdom's AbortController to be used with undici's fetch.

KhafraDev · 2023-03-10T23:02:17Z

So it does seem like an issue in vitest - overriding some globals is bound to cause issues (see jest...). Although we should offer support for 3rd party AbortControllers, so this is a bug. We also shouldn't be changing the max listeners if the amount was changed, but there's no way of checking so there isn't much to do with that.

camsjams · 2023-03-21T17:55:29Z

The heavily used third-party WHATWG implementation of an AbortController interface called abort-controller, also fails the test for getEventListeners.

For those that can, just update to use native one: https://nodejs.org/dist/latest-v18.x/docs/api/globals.html#class-abortcontroller

KhafraDev · 2023-03-21T18:11:13Z

this was already fixed in undici v5.21.0, node v19.6.0 is using undici v5.19.1.

camsjams · 2023-03-21T18:12:32Z

So does mainline Node have to update?

KhafraDev · 2023-03-21T18:14:58Z

node v19.8+ has the fix

camsjams · 2023-03-21T18:30:41Z

Sorry by mainline I guess I meant LTS (Node 18)

sangaman · 2023-03-25T01:39:49Z

node v19.8+ has the fix

I'm still seeing this MaxListenersExceededWarning with AbortSignal on v19.8.1, it appears to be triggered by calling the native fetch a handful of times in a test suite.

himself65 · 2023-04-07T02:40:44Z

is there any hotpatch for nodejs 18?

ba55ie · 2023-04-07T14:18:44Z

is there any hotpatch for nodejs 18?

You can patch globalThis.fetch with the latest version from undici:

import { fetch } from 'undici';

globalThis.fetch = fetch;

That's how I fixed our tests in node v18.15.0:

import { fetch } from 'undici';
import { beforeAll, afterAll, test } from 'your-test-lib';

let originalFetch;

beforeAll(() => {
  originalFetch = globalThis.fetch;
  globalThis.fetch = fetch;
});

afterAll(() => {
  globalThis.fetch = originalFetch;
});

// Run your tests using fetch.

IanVS · 2023-04-14T19:03:14Z

Looks like this is now included in node 18.16.0 as well. 👍

Addresses: nodejs/undici#1910

* chore(ftux): full integration test * chore: update node for tests Addresses: nodejs/undici#1910

thw0rted · 2023-08-13T18:15:34Z

I'm still seeing this with Jest tests on Node 18.16.1:

(node:10904) MaxListenersExceededWarning: Possible EventTarget memory leak detected. 11 abort listeners added to [AbortSignal]. Use events.setMaxListeners() to increase limit

Is this PR supposed to address the warning automatically, or am I meant to be manually increasing setMaxListeners?

Maxlitv · 2023-09-06T14:12:21Z

Looks like this is now included in node 18.16.0 as well. 👍

I love you.

fix(fetch): raise AbortSignal max event listeners

7615308

ronag approved these changes Feb 6, 2023

View reviewed changes

mcollina approved these changes Feb 7, 2023

View reviewed changes

ronag merged commit 7827031 into nodejs:main Feb 7, 2023

KhafraDev deleted the raise-abort-listener-max branch February 7, 2023 14:23

timdorr mentioned this pull request Feb 20, 2023

[Bug]: Type error with newest node version 19.6.1 due to update of undici to version 5.19.1 remix-run/react-router#10118

Closed

This was referenced Mar 10, 2023

fix(fetch): third party abortcontrollers throwing errors #2002

Merged

events: add getMaxListeners method nodejs/node#47039

Merged

This was referenced Mar 21, 2023

fetch errors when providing an abort signal nodejs/node#46823

Open

Broken on node 18.14.2 and 19.16.1 mysticatea/abort-controller#39

Open

timdorr mentioned this pull request Mar 28, 2023

[Bug]: createMemoryRouter in test environment fails with node 18.15.0 remix-run/react-router#10273

Closed

anonrig pushed a commit to anonrig/undici that referenced this pull request Apr 4, 2023

fix(fetch): raise AbortSignal max event listeners (nodejs#1910)

ca0423a

fawazahmed0 mentioned this pull request Apr 6, 2023

MaxListenersExceededWarning: Possible EventTarget memory leak detected. 101 abort listeners added to [AbortSignal] nodejs/node#47449

Closed

himself65 mentioned this pull request Apr 7, 2023

TypeError: The "emitter" argument must be an instance of EventEmitter or EventTarget. Received an instance of AbortSignal sindresorhus/ky#498

Closed

metcoder95 mentioned this pull request Apr 12, 2023

feat: Add H2 support metcoder95/undici#33

Closed

7 tasks

neurosnap added a commit to aptible/app-ui that referenced this pull request May 4, 2023

chore: update node for tests

cabd2d4

Addresses: nodejs/undici#1910

neurosnap added a commit to aptible/app-ui that referenced this pull request May 8, 2023

chore: update node for tests

c1b7398

Addresses: nodejs/undici#1910

neurosnap added a commit to aptible/app-ui that referenced this pull request May 9, 2023

A single ftux integration test (#84)

404567f

* chore(ftux): full integration test * chore: update node for tests Addresses: nodejs/undici#1910

This was referenced Jun 22, 2023

2305 document Node 18.16-or-later requirement stoplightio/prism#2314

Merged

Prism does not start mock locally stoplightio/prism#2305

Closed

metcoder95 pushed a commit to metcoder95/undici that referenced this pull request Jul 21, 2023

fix(fetch): raise AbortSignal max event listeners (nodejs#1910)

023b560

genaris mentioned this pull request Feb 8, 2024

Error: The "emitter" argument must be an instance of EventEmitter openwallet-foundation/credo-ts#1751

Closed

This was referenced Feb 12, 2024

MSW fails to recognize a registered handler mswjs/msw#2012

Closed

Stop polyfilling existing globals jsdom/jsdom#3682

Closed

crysmags pushed a commit to crysmags/undici that referenced this pull request Feb 27, 2024

fix(fetch): raise AbortSignal max event listeners (nodejs#1910)

1a63bf2

JoshMock mentioned this pull request Feb 28, 2024

MaxListenersExceededWarning: Possible EventEmitter memory leak detected. 11 abort listeners added to [EventEmitter] elastic/elastic-transport-js#63

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(fetch): raise AbortSignal max event listeners #1910

fix(fetch): raise AbortSignal max event listeners #1910

KhafraDev commented Feb 6, 2023

codecov-commenter commented Feb 6, 2023 •

edited

Loading

mcollina left a comment •

edited

Loading

ricardocosta commented Mar 8, 2023

KhafraDev commented Mar 8, 2023

joshkel commented Mar 10, 2023

KhafraDev commented Mar 10, 2023

camsjams commented Mar 21, 2023 •

edited

Loading

KhafraDev commented Mar 21, 2023

camsjams commented Mar 21, 2023

KhafraDev commented Mar 21, 2023 •

edited

Loading

camsjams commented Mar 21, 2023

sangaman commented Mar 25, 2023

himself65 commented Apr 7, 2023

ba55ie commented Apr 7, 2023

IanVS commented Apr 14, 2023

thw0rted commented Aug 13, 2023

Maxlitv commented Sep 6, 2023

fix(fetch): raise AbortSignal max event listeners #1910

fix(fetch): raise AbortSignal max event listeners #1910

Conversation

KhafraDev commented Feb 6, 2023

codecov-commenter commented Feb 6, 2023 • edited Loading

Codecov Report

mcollina left a comment • edited Loading

Choose a reason for hiding this comment

ricardocosta commented Mar 8, 2023

KhafraDev commented Mar 8, 2023

joshkel commented Mar 10, 2023

KhafraDev commented Mar 10, 2023

camsjams commented Mar 21, 2023 • edited Loading

KhafraDev commented Mar 21, 2023

camsjams commented Mar 21, 2023

KhafraDev commented Mar 21, 2023 • edited Loading

camsjams commented Mar 21, 2023

sangaman commented Mar 25, 2023

himself65 commented Apr 7, 2023

ba55ie commented Apr 7, 2023

IanVS commented Apr 14, 2023

thw0rted commented Aug 13, 2023

Maxlitv commented Sep 6, 2023

codecov-commenter commented Feb 6, 2023 •

edited

Loading

mcollina left a comment •

edited

Loading

camsjams commented Mar 21, 2023 •

edited

Loading

KhafraDev commented Mar 21, 2023 •

edited

Loading